feat!(ServiceProviderRegistry): add ipniPeerId to PDPOffering #266
Conversation
Yes, we only need 1. MKv2 uses a single peerID per provider to publish all ads. PDPV0 only seem to publish IPFS so this will work for both. |
github-project-automation
bot
moved this from 🔎 Awaiting review
to ✔️ Approved by reviewer
in FS
| minProvingPeriodInEpochs: 2880, | ||
| location: "US-Central", | ||
| paymentTokenAddress: IERC20(address(0)) // Payment in FIL | ||
| paymentTokenAddress: IERC20(address(0)), // Payment in FIL |
There was a problem hiding this comment.
make this a constant like
IERC20 constant NATIVE_FIL = IERC20(address(0))
can declare it outside of the test contract
|
Just trying to plan a bit ahead, does this change require a new deployment of the ServiceProviderRegistry? |
| uint256 minProvingPeriodInEpochs; // Minimum proving period in epochs | ||
| string location; // Geographic location of the service provider | ||
| IERC20 paymentTokenAddress; // Token contract for payment (IERC20(address(0)) for FIL) | ||
| string ipniPeerId; // IPNI peer ID as CID string (max 256 chars, can be empty) |
There was a problem hiding this comment.
could some of these be capabilityKeys instead?
There was a problem hiding this comment.
My original plan was to push more into capabilities and have fewer opinions about this object struct, what we've ended up with is as a result of negotiation.
But where we landed was treating the struct as "schema of things we know we'll want" and capabilities as "unknowns" then this one fits in the struct.
There was a problem hiding this comment.
Broader discussion here: #297
I think INPI makes sense as a capability, and we should probably use capabilities for all optional fields.
|
@rjan90 yes it does. |
|
Although technically I don't think this is breaking and could be upgraded in the implementation and we wouldn't need a new contract deployment, I think that's right @wjmelements? If we add to the end then it's an upgrade and existing clients can still abi.decode into the rest of the struct and just ignore this? |
It breaks the ABI of Appending to structs is generally upgrade-safe for proxy storage layouts. However, essentially all fields are permanent after that. |
Upon further review, this isn't appending to a struct but to encoded |
wjmelements
changed the title
|
this has been rolled back to as long as we have |
That makes sense because these fields are related. I still think they could be bundled into an IPNI capability. Alternatively I was thinking these |
|
superceded by #308 |
Closes: #226
@LexLuthr we're only going to need one of these, right? We're not going to use a second one if we advertise PieceCID as well?
I'm thinking that we can use this to do a cross-reference with cid.contact that the SP has actually published something. A check for the root CID will have to be enough, and is most likely sufficient for most use cases anyway.